Separate explain mode from format params #5042
Conversation
…ation Signed-off-by: Heng Qian <[email protected]>
qianheng-aws
requested review from
GumpacG,
LantaoJin,
MaxKsyunz,
RyanL1997,
Swiddis,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
noCharger,
penghuo,
ps48,
seankao-az,
vamsimanohar,
ykmr1224 and
yuancu
as code owners
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplace Explain's previous format enum with a new ExplainMode enum and propagate explain-mode handling across AST, plan creation, execution, PPL request/transport, plugin, protocol formats, tests, and integration test expected outputs (mode vs. serialization format separated). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant REST_Plugin
participant Transport
participant PPLService
participant AstBuilder
participant ExecutionEngine
participant OpenSearch
Client->>REST_Plugin: HTTP explain request (format=<json|yaml>, mode=<simple|standard|extended|cost>)
REST_Plugin->>Transport: build TransportPPLQueryRequest(format, explainMode)
Transport->>PPLService: deliver PPLQueryRequest(with explainMode)
PPLService->>AstBuilder: build AST (context.explainMode)
AstBuilder->>ExecutionEngine: explain(plan, ExplainMode)
ExecutionEngine->>OpenSearch: run explain with ExplainMode
OpenSearch-->>ExecutionEngine: explain result
ExecutionEngine-->>REST_Plugin: ExplainResponse (serialized per format)
REST_Plugin-->>Client: HTTP response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ppl/src/main/java/org/opensearch/sql/ppl/parser/AstStatementBuilder.java (1)
50-57: Fix missingexplainModeinitialization in multipleStatementBuilderContextcallers.The
explainModefield is accessed at line 41 whenisExplain=truebut is not set by several callers:
SQLService.java:99-103(production code)CalcitePPLAbstractTest.java:125UnifiedQueryPlanner.java:81AstStatementBuilderTest.java:75PPLQueryDataAnonymizerTest.java:956Only
PPLService.java:98-102properly sets the field. Update all other builders to include.explainMode(...)or provide a sensible default value.legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java (1)
15-37: Add JavaDoc to public enum and methods.The removal of explain-related formats (SIMPLE, STANDARD, EXTENDED, COST) and the
EXPLAIN_FORMATSmap is correct and aligns with moving explain mode handling elsewhere. The legacyFormatenum now correctly focuses only on response formats (JDBC, CSV, RAW, TABLE), and there are no remaining references in the legacy module to the removed constants.However, the public enum
Format, public methodof(), and public static fieldRESPONSE_FORMATSlack JavaDoc documentation. Per coding guidelines, all public classes and methods must have proper JavaDoc.
🤖 Fix all issues with AI agents
In @integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java:
- Around line 61-65: The deprecation Javadoc for explainQueryToString(String)
incorrectly directs users to explainQueryYaml(String) despite this method
returning JSON; update the deprecation notice to reference the correct
overloaded method explainQueryToString(String, ExplainMode) (or clarify it
returns JSON) so callers are directed to the JSON-capable alternative; modify
the Javadoc line above the method (and the @Deprecated tag comment) to point to
{@link #explainQueryToString(String, ExplainMode)}.
- Around line 79-84: The explainQuery method constructs the EXPLAIN API path
using enum toString() values which yield uppercase names; replace the enum
arguments in the String.format call with the proper lowercase getters (use
Format.getFormatName() and ExplainMode.getModeName()) so
String.format(EXPLAIN_API_ENDPOINT, format.getFormatName(), mode.getModeName())
is used instead of passing format and mode directly; update the call in the
explainQuery method to reference these getters and remove reliance on
toString().
🧹 Nitpick comments (9)
core/src/test/java/org/opensearch/sql/executor/execution/ExplainPlanTest.java (1)
35-35: Consider using a concreteExplainModeenum value instead of mocking.Mocking an enum is unconventional and can lead to unexpected behavior. Since
ExplainModeis likely a simple enum, using a concrete value likeExplainMode.SIMPLEorExplainMode.STANDARDwould be clearer and more idiomatic.Suggested change
- @Mock private ExplainMode mode; + private ExplainMode mode = ExplainMode.SIMPLE;Then remove
@Mockand use the concrete enum value directly in test methods.core/src/main/java/org/opensearch/sql/executor/execution/AbstractPlan.java (1)
28-34: Add@param modeto the JavaDoc.Per coding guidelines, public methods must have complete JavaDoc with
@paramfor all parameters. Themodeparameter is undocumented.Suggested JavaDoc update
/** * Explain query execution. * * @param listener query explain response listener. + * @param mode the explain mode determining output detail level. */ public abstract void explain( ResponseListener<ExecutionEngine.ExplainResponse> listener, ExplainMode mode);core/src/main/java/org/opensearch/sql/ast/statement/ExplainMode.java (1)
12-29: Consider catching specific exception and adding JavaDoc.The enum design is clean and the use of
Locale.ROOTfor case-insensitive matching is good practice. Two suggestions:
Catch specific exception:
valueOf()throwsIllegalArgumentExceptionfor invalid names. Catching the genericExceptionis overly broad and could mask unexpected errors.Missing JavaDoc: Per coding guidelines, public classes and methods in
core/src/main/javashould have JavaDoc with@param,@returndocumentation.Suggested improvements
+/** + * Defines the explain output mode for query plan visualization. + */ @RequiredArgsConstructor public enum ExplainMode { SIMPLE("simple"), STANDARD("standard"), EXTENDED("extended"), COST("cost"); @Getter private final String modeName; + /** + * Converts a string mode name to an ExplainMode enum value. + * + * @param mode the mode name (case-insensitive), may be null + * @return the corresponding ExplainMode, defaults to STANDARD if null, empty, or invalid + */ public static ExplainMode of(String mode) { if (mode == null || mode.isEmpty()) return STANDARD; try { return ExplainMode.valueOf(mode.toUpperCase(Locale.ROOT)); - } catch (Exception e) { + } catch (IllegalArgumentException e) { return ExplainMode.STANDARD; } } }core/src/main/java/org/opensearch/sql/executor/execution/ExplainPlan.java (1)
22-33: Consider adding JavaDoc to the constructor.As per coding guidelines for
core/src/main/java/**/*.java, public methods should have JavaDoc with@param,@return, and@throwsannotations. The constructor currently has a minimal comment.📝 Suggested JavaDoc
- /** Constructor. */ + /** + * Constructs an ExplainPlan that wraps another plan for explanation. + * + * @param queryId the unique query identifier + * @param queryType the type of query (SQL or PPL) + * @param plan the underlying plan to explain + * @param mode the explain mode (SIMPLE, STANDARD, EXTENDED, or COST) + * @param explainListener listener for the explain response + */ public ExplainPlan(legacy/src/main/java/org/opensearch/sql/legacy/request/SqlRequestParam.java (1)
42-54: Remove unusedpathparameter.The
pathparameter is not used in the method body and all callers invoke the single-parameter overload that passesnull. Simplify by removing this unused parameter.♻️ Suggested cleanup
- public static Format getFormat(Map<String, String> requestParams, String path) { + public static Format getFormat(Map<String, String> requestParams) { String formatName = requestParams.containsKey(QUERY_PARAMS_FORMAT) ? requestParams.get(QUERY_PARAMS_FORMAT).toLowerCase() : DEFAULT_RESPONSE_FORMAT; Optional<Format> optionalFormat = Format.of(formatName);Also remove the overload method
getFormat(Map<String, String> requestParams)that currently just delegates to this method withnull.ppl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.java (1)
93-95: Missing JavaDoc on public method.Per coding guidelines, public methods should have JavaDoc with
@returndocumentation.📝 Suggested JavaDoc
+ /** + * Convert the explain mode string to an ExplainMode instance. + * + * @return the ExplainMode corresponding to the explainMode string + */ public ExplainMode mode() { return ExplainMode.of(explainMode); }plugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.java (1)
137-144: Add JavaDoc for new private method.While private methods don't strictly require JavaDoc, adding a brief comment explaining the null return for non-explain requests would improve maintainability.
📝 Suggested documentation
+ /** + * Get explain mode from request parameters. + * + * @return the explain mode string, or null if not an explain request + */ private static String getExplainMode(Map<String, String> requestParams, String path) { if (!isExplainRequest(path)) { return null; } return requestParams .getOrDefault(QUERY_PARAMS_EXPLAIN_MODE, DEFAULT_EXPLAIN_MODE) .toLowerCase(); }core/src/main/java/org/opensearch/sql/executor/QueryService.java (2)
204-207: Minor style: redundant parentheses.The parentheses around
(mode != ExplainMode.STANDARD)are unnecessary.📝 Suggested simplification
- if (mode != null && (mode != ExplainMode.STANDARD)) { + if (mode != null && mode != ExplainMode.STANDARD) { throw new UnsupportedOperationException( "Explain mode " + mode.name() + " is not supported in v2 engine"); }
188-202: Missing@paramformodeparameter in JavaDoc.The JavaDoc should document the new
modeparameter.📝 Suggested JavaDoc update
/** * Explain the query in {@link UnresolvedPlan} using {@link ResponseListener} to get and format * explain response. * * @param plan {@link UnresolvedPlan} * @param queryType {@link QueryType} * @param listener {@link ResponseListener} for explain response + * @param mode {@link ExplainMode} for explain output format (only STANDARD supported in v2) * @param calciteFailure Optional failure thrown from calcite */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
core/src/main/java/org/opensearch/sql/ast/statement/Explain.javacore/src/main/java/org/opensearch/sql/ast/statement/ExplainMode.javacore/src/main/java/org/opensearch/sql/executor/ExecutionEngine.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/executor/execution/AbstractPlan.javacore/src/main/java/org/opensearch/sql/executor/execution/CommandPlan.javacore/src/main/java/org/opensearch/sql/executor/execution/ExplainPlan.javacore/src/main/java/org/opensearch/sql/executor/execution/QueryPlan.javacore/src/main/java/org/opensearch/sql/executor/execution/QueryPlanFactory.javacore/src/test/java/org/opensearch/sql/executor/execution/ExplainPlanTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.javainteg-test/src/test/java/org/opensearch/sql/util/MatcherUtils.javainteg-test/src/test/resources/expectedOutput/calcite/explain_extended_for_standardization.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_extended_for_standardization.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_cost.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_output_cost.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_extended.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_output_extended.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_simple.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_output_simple.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_standard.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_output_standard.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_skip_script_encoding.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_skip_script_encoding.yamllegacy/src/main/java/org/opensearch/sql/legacy/executor/Format.javalegacy/src/main/java/org/opensearch/sql/legacy/request/SqlRequestParam.javaopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javaplugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.javaplugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.javaplugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryRequest.javappl/src/main/java/org/opensearch/sql/ppl/PPLService.javappl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.javappl/src/main/java/org/opensearch/sql/ppl/parser/AstStatementBuilder.javappl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.javaprotocol/src/main/java/org/opensearch/sql/protocol/response/format/Format.javaprotocol/src/test/java/org/opensearch/sql/protocol/response/format/FormatTest.java
💤 Files with no reviewable changes (2)
- integ-test/src/test/resources/expectedOutput/calcite/explain_extended_for_standardization.json
- integ-test/src/test/resources/expectedOutput/calcite/explain_skip_script_encoding.json
🧰 Additional context used
📓 Path-based instructions (10)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
protocol/src/test/java/org/opensearch/sql/protocol/response/format/FormatTest.javacore/src/main/java/org/opensearch/sql/executor/execution/AbstractPlan.javacore/src/main/java/org/opensearch/sql/executor/ExecutionEngine.javacore/src/main/java/org/opensearch/sql/executor/execution/QueryPlan.javacore/src/main/java/org/opensearch/sql/executor/execution/CommandPlan.javalegacy/src/main/java/org/opensearch/sql/legacy/executor/Format.javacore/src/main/java/org/opensearch/sql/ast/statement/ExplainMode.javalegacy/src/main/java/org/opensearch/sql/legacy/request/SqlRequestParam.javacore/src/main/java/org/opensearch/sql/executor/execution/QueryPlanFactory.javaprotocol/src/main/java/org/opensearch/sql/protocol/response/format/Format.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javappl/src/main/java/org/opensearch/sql/ppl/PPLService.javacore/src/main/java/org/opensearch/sql/executor/execution/ExplainPlan.javaplugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryRequest.javappl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.javappl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.javaopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javacore/src/test/java/org/opensearch/sql/executor/execution/ExplainPlanTest.javacore/src/main/java/org/opensearch/sql/ast/statement/Explain.javaplugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.javainteg-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.javaplugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.javainteg-test/src/test/java/org/opensearch/sql/util/MatcherUtils.javappl/src/main/java/org/opensearch/sql/ppl/parser/AstStatementBuilder.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
protocol/src/test/java/org/opensearch/sql/protocol/response/format/FormatTest.javacore/src/main/java/org/opensearch/sql/executor/execution/AbstractPlan.javacore/src/main/java/org/opensearch/sql/executor/ExecutionEngine.javacore/src/main/java/org/opensearch/sql/executor/execution/QueryPlan.javacore/src/main/java/org/opensearch/sql/executor/execution/CommandPlan.javalegacy/src/main/java/org/opensearch/sql/legacy/executor/Format.javacore/src/main/java/org/opensearch/sql/ast/statement/ExplainMode.javalegacy/src/main/java/org/opensearch/sql/legacy/request/SqlRequestParam.javacore/src/main/java/org/opensearch/sql/executor/execution/QueryPlanFactory.javaprotocol/src/main/java/org/opensearch/sql/protocol/response/format/Format.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javappl/src/main/java/org/opensearch/sql/ppl/PPLService.javacore/src/main/java/org/opensearch/sql/executor/execution/ExplainPlan.javaplugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryRequest.javappl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.javappl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.javaopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javacore/src/test/java/org/opensearch/sql/executor/execution/ExplainPlanTest.javacore/src/main/java/org/opensearch/sql/ast/statement/Explain.javaplugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.javainteg-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.javaplugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.javainteg-test/src/test/java/org/opensearch/sql/util/MatcherUtils.javappl/src/main/java/org/opensearch/sql/ppl/parser/AstStatementBuilder.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
**/*Test.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*Test.java: All new business logic requires unit tests
Name unit tests with*Test.javasuffix in OpenSearch SQL
Files:
protocol/src/test/java/org/opensearch/sql/protocol/response/format/FormatTest.javacore/src/test/java/org/opensearch/sql/executor/execution/ExplainPlanTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
protocol/src/test/java/org/opensearch/sql/protocol/response/format/FormatTest.javacore/src/test/java/org/opensearch/sql/executor/execution/ExplainPlanTest.javainteg-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.javainteg-test/src/test/java/org/opensearch/sql/util/MatcherUtils.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
integ-test/src/test/resources/**/*
⚙️ CodeRabbit configuration file
integ-test/src/test/resources/**/*: - Verify test data is realistic and representative
- Check data format matches expected schema
- Ensure test data covers edge cases and boundary conditions
Files:
integ-test/src/test/resources/expectedOutput/calcite/explain_skip_script_encoding.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_simple.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_standard.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_output_extended.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_extended_for_standardization.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_standard.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_extended.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_cost.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_simple.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_output_cost.json
core/src/main/java/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/**/*.java: - New functions MUST have unit tests in the same commit
Files:
core/src/main/java/org/opensearch/sql/executor/execution/AbstractPlan.javacore/src/main/java/org/opensearch/sql/executor/ExecutionEngine.javacore/src/main/java/org/opensearch/sql/executor/execution/QueryPlan.javacore/src/main/java/org/opensearch/sql/executor/execution/CommandPlan.javacore/src/main/java/org/opensearch/sql/ast/statement/ExplainMode.javacore/src/main/java/org/opensearch/sql/executor/execution/QueryPlanFactory.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/executor/execution/ExplainPlan.javacore/src/main/java/org/opensearch/sql/ast/statement/Explain.java
core/src/main/java/org/opensearch/sql/ast/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/org/opensearch/sql/ast/**/*.java: - AST nodes must be immutable where possible
- Follow visitor pattern for AST traversal
- Ensure proper toString() implementation for debugging
Files:
core/src/main/java/org/opensearch/sql/ast/statement/ExplainMode.javacore/src/main/java/org/opensearch/sql/ast/statement/Explain.java
**/ppl/**/*.java
⚙️ CodeRabbit configuration file
**/ppl/**/*.java: - For PPL parser changes, verify grammar tests with positive/negative cases
- Check AST generation for new syntax
- Ensure corresponding AST builder classes are updated
- Validate edge cases and boundary conditions
Files:
ppl/src/main/java/org/opensearch/sql/ppl/PPLService.javappl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.javappl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.javainteg-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.javappl/src/main/java/org/opensearch/sql/ppl/parser/AstStatementBuilder.java
integ-test/**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
End-to-end scenarios need integration tests in
integ-test/module
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
⚙️ CodeRabbit configuration file
integ-test/**/*IT.java: - Integration tests MUST use valid test data from resources
- Verify test data files exist in integ-test/src/test/resources/
- Check test assertions are meaningful and specific
- Validate tests clean up resources after execution
- Ensure tests are independent and can run in any order
- Flag tests that reference non-existent indices (e.g., EMP)
- Verify integration tests are in correct module (integ-test/)
- Check tests can be run with ./gradlew :integ-test:integTest
- Ensure proper test data setup and teardown
- Validate end-to-end scenario coverage
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Name integration tests with
*IT.javasuffix in OpenSearch SQL
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
**/calcite/**/*.java
⚙️ CodeRabbit configuration file
**/calcite/**/*.java: - Follow existing Calcite integration patterns
- Verify RelNode visitor implementations are complete
- Check RexNode handling follows project conventions
- Validate SQL generation is correct and optimized
- Ensure Calcite version compatibility
- Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
🧠 Learnings (8)
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
integ-test/src/test/resources/expectedOutput/calcite/explain_skip_script_encoding.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_simple.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_standard.jsoncore/src/main/java/org/opensearch/sql/executor/ExecutionEngine.javainteg-test/src/test/resources/expectedOutput/calcite/explain_output_extended.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_extended_for_standardization.yamlcore/src/main/java/org/opensearch/sql/executor/QueryService.javainteg-test/src/test/resources/expectedOutput/calcite/explain_output_standard.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_extended.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_cost.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_simple.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_output_cost.jsonopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javacore/src/test/java/org/opensearch/sql/executor/execution/ExplainPlanTest.javainteg-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
📚 Learning: 2025-12-11T05:27:39.856Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 0
File: :0-0
Timestamp: 2025-12-11T05:27:39.856Z
Learning: In opensearch-project/sql, for SEMI and ANTI join types in CalciteRelNodeVisitor.java, the `max` option has no effect because these join types only use the left side to filter records based on the existence of matches in the right side. The join results are identical regardless of max value (max=1, max=2, or max=∞). The early return for SEMI/ANTI joins before processing the `max` option is intentional and correct behavior.
Applied to files:
integ-test/src/test/resources/expectedOutput/calcite/explain_skip_script_encoding.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_simple.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_extended.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_output_standard.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_extended.yaml
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
Applied to files:
integ-test/src/test/resources/expectedOutput/calcite/explain_output_simple.yamlcore/src/main/java/org/opensearch/sql/executor/ExecutionEngine.javacore/src/main/java/org/opensearch/sql/executor/execution/QueryPlanFactory.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/ast/statement/Explain.java
📚 Learning: 2025-12-29T05:32:11.893Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:11.893Z
Learning: In opensearch-project/sql, when creating custom Calcite RelNode classes that extend EnumerableLimitSort or other Calcite RelNode types, always override the `copy` method. Without overriding copy, the class will downgrade to its parent class type during copy operations, losing the custom implementation.
Applied to files:
integ-test/src/test/resources/expectedOutput/calcite/explain_output_simple.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_extended.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_output_extended.yaml
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Document Calcite-specific workarounds in code
Applied to files:
integ-test/src/test/resources/expectedOutput/calcite/explain_output_extended.json
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Update corresponding AST builder classes when making PPL grammar changes
Applied to files:
ppl/src/main/java/org/opensearch/sql/ppl/PPLService.javappl/src/main/java/org/opensearch/sql/ppl/parser/AstStatementBuilder.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
📚 Learning: 2025-12-29T05:32:03.491Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:03.491Z
Learning: For any custom Calcite RelNode class (e.g., ones that extend EnumerableLimitSort or other Calcite RelNode types), always override the copy method. If copy is not overridden, cloning/copy operations may downgrade the instance to the parent class type, losing the custom behavior. In your implementation, ensure copy returns a new instance of the concrete class with all relevant fields and traits preserved, mirroring the current instance state.
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
🧬 Code graph analysis (2)
plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryRequest.java (1)
core/src/main/java/org/opensearch/sql/ast/statement/Explain.java (1)
Getter(14-36)
ppl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.java (1)
core/src/main/java/org/opensearch/sql/ast/statement/Explain.java (1)
Getter(14-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: security-it-linux (25)
- GitHub Check: security-it-linux (21)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (21, unit)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (25, doc)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: build-linux (25, integration)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: test-sql-cli-integration (21)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: WhiteSource Security Check
🔇 Additional comments (45)
integ-test/src/test/java/org/opensearch/sql/util/MatcherUtils.java (1)
487-488: LGTM!The addition of ID normalization in
cleanUpYamlfollows the established pattern for handling dynamic values in test output. The regexid = \\d+correctly matches numeric IDs with the expected spacing and normalizes them toid = *for stable test comparisons across different explain modes.integ-test/src/test/resources/expectedOutput/calcite/explain_skip_script_encoding.yaml (1)
1-8: LGTM! Test resource correctly captures expected explain output.The YAML structure is well-formed with consistent logical and physical plan representations:
- Logical plan accurately reflects the query semantics (filter on address equality and arithmetic age condition)
- Physical plan shows proper pushdown context with PROJECT, SCRIPT, and LIMIT operations
- Script parameters (DIGESTS/SOURCES) correctly map to the filter operands
- Hardcoded
utcTimestamp: 0ensures deterministic test outputThe test data is realistic and covers meaningful edge cases including string comparison, arithmetic expressions in filters, and multi-field projection pushdown.
integ-test/src/test/resources/expectedOutput/calcite/explain_output_simple.yaml (1)
1-14: LGTM!The YAML structure is valid and the logical plan hierarchy appropriately represents a simple explain output for Calcite, showing only node names without detailed attributes. The plan structure (SystemLimit → Project → Filter → Sort → Aggregate → Filter → IndexScan) is realistic for a typical query.
integ-test/src/test/resources/expectedOutput/calcite/explain_extended_for_standardization.yaml (1)
1-9: LGTM!The extended explain output correctly includes both logical and physical plan sections with appropriate detail levels. The logical section shows typed attributes (fetch, type, expressions), and the physical section includes the full
PushDownContextandOpenSearchRequestBuilderdetails expected for extended mode verification.protocol/src/test/java/org/opensearch/sql/protocol/response/format/FormatTest.java (1)
53-58: LGTM!The test correctly reflects the updated default behavior where
Format.ofExplain("")now returnsFormat.JSONinstead of the previousFormat.STANDARD. This aligns with the PR's separation of explain mode from format parameters, where format controls output serialization (JSON/YAML) and mode controls verbosity level.integ-test/src/test/resources/expectedOutput/calcite/explain_output_simple.json (1)
1-5: LGTM!The JSON test resource correctly mirrors the YAML version with identical logical plan content, validating that the same simple explain mode produces consistent output across different formats.
ppl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.java (1)
152-157: LGTM!The change from
node.getFormat()tonode.getMode()correctly aligns the anonymizer with the newExplainModeAPI. The output will now reflect the explain mode (e.g., "explain simple", "explain extended") in anonymized queries.plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java (1)
77-82: LGTM!Adding "mode" to the response parameters correctly enables the explain mode query parameter to be processed in PPL REST requests, supporting the new separation of mode from format.
ppl/src/main/java/org/opensearch/sql/ppl/PPLService.java (1)
98-102: LGTM!The addition of
.explainMode(request.getExplainMode())correctly propagates the explain mode through the AST building context, following the established builder pattern. WhilegetExplainMode()can return null when not an explain request, this is safely handled downstream: theExplainclass constructor callsExplainMode.of(mode), which defaults toSTANDARDwhen null is passed.core/src/test/java/org/opensearch/sql/executor/execution/ExplainPlanTest.java (2)
37-45: Test correctly verifies the explain execution flow.The test properly validates that
ExplainPlan.execute()delegates toqueryPlan.explain()with the correct listener and mode parameters.
47-58: Exception test validates unsupported explain-on-explain scenario.The test correctly verifies that calling
explain()on anExplainPlanthrowsUnsupportedOperationExceptionwith the expected message.integ-test/src/test/resources/expectedOutput/calcite/explain_output_standard.yaml (1)
1-21: Test resource correctly defines expected STANDARD mode explain output.The YAML structure properly separates logical and physical plans for the Calcite explain output. The test data is realistic and aligns with the mode-based explain testing infrastructure introduced in this PR.
ppl/src/main/java/org/opensearch/sql/ppl/parser/AstStatementBuilder.java (1)
40-42: Correctly separates explain mode from format in Explain construction.The change properly uses
context.explainModeinstead ofcontext.formatwhen building theExplainstatement, achieving the PR's objective of separating explain mode from format parameters.integ-test/src/test/resources/expectedOutput/calcite/explain_output_standard.json (1)
1-6: Test resource correctly defines expected STANDARD mode explain output in JSON format.The JSON structure is valid and content is consistent with the corresponding YAML resource. This enables testing the same explain mode across different output formats.
integ-test/src/test/resources/expectedOutput/calcite/explain_output_extended.json (1)
1-7: LGTM - Extended explain output test resource.The test resource correctly captures all three sections (logical, physical, extended) expected from the extended explain mode. The extended section appropriately includes the Java-based bind implementation and iterator logic that Calcite generates.
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java (4)
28-29: LGTM - Import changes for ExplainMode usage.The imports are correctly added for the new
ExplainModeenum andSettings.Keyused in the test methods.
381-389: LGTM - Updated to use ExplainMode with explicit format.The test correctly separates the explain mode (EXTENDED) from the format (YAML), demonstrating the new API pattern introduced in this PR.
2209-2219: LGTM - Consistent use of ExplainMode.EXTENDED.The test correctly uses the new
ExplainMode.EXTENDEDparameter and loads the appropriate expected output file.
2322-2347: Good test coverage for all explain modes and formats.This test comprehensively validates the separation of explain mode from format by iterating through all
ExplainModevalues (SIMPLE, STANDARD, EXTENDED, COST) and testing both YAML and JSON output formats for each mode. The helper methodsexplainQueryYaml(query, ExplainMode)andexplainQueryToString(query, ExplainMode)are available in the parentExplainITclass, and corresponding test resource files exist atinteg-test/src/test/resources/expectedOutput/calcite/.integ-test/src/test/resources/expectedOutput/calcite/explain_output_cost.json (1)
1-6: LGTM - Cost explain output test resource.The test resource correctly includes per-operator cost statistics (rowcount, cumulative cost, cpu, io, id) in both logical and physical plan sections, which is the expected format for
ExplainMode.COST. The absence of an "extended" section is appropriate for the cost mode.protocol/src/main/java/org/opensearch/sql/protocol/response/format/Format.java (2)
54-57: Default explain format changed from "standard" to "json".The default format is now
"json"instead of"standard". This makes sense since the Format enum now represents output formats (JSON/YAML), not explain modes.
21-26: Clean separation of format from mode - refactoring verified as complete.The refactoring correctly removes explain modes (SIMPLE, STANDARD, EXTENDED, COST) from the Format enum, leaving only output formats (JSON, YAML). This aligns with the PR objective of separating explain mode from format parameters.
No external usages of the removed Format constants exist in the codebase—all references have been migrated to the ExplainMode enum. The default explain format has been updated to "json" as intended.
core/src/main/java/org/opensearch/sql/executor/ExecutionEngine.java (2)
16-16: LGTM - Import updated to ExplainMode.The import correctly references the new
ExplainModeclass that replacesExplain.ExplainFormat.
54-58: Verify implementation consistency across all ExecutionEngine implementations.The interface method correctly uses
ExplainModeas a parameter type (lines 54-58). However, manual verification is needed to ensure all implementing classes have been updated consistently. The proposed shell script should be executed to identify all classes implementingExecutionEngineand cross-check theirexplain()method signatures to confirm they all reflect the same parameter type change.core/src/main/java/org/opensearch/sql/executor/execution/CommandPlan.java (1)
8-8: LGTM! Clean migration to ExplainMode.The import and method signature updates are consistent with the new
ExplainModeabstraction. The behavior (throwingUnsupportedOperationException) is appropriately preserved.Minor note: Per coding guidelines, public methods should have JavaDoc with
@paramand@throwsannotations. Consider adding documentation to theexplainmethod.Also applies to: 48-52
core/src/main/java/org/opensearch/sql/executor/execution/QueryPlan.java (1)
10-10: LGTM! Correct migration to ExplainMode.The import update, method signature change, and forwarding of
modetoqueryService.explain()are all consistent with the refactoring. The pagination guard and error handling remain correctly in place.Also applies to: 70-80
integ-test/src/test/resources/expectedOutput/calcite/explain_output_cost.yaml (1)
1-21: Good test coverage for COST explain mode.The expected output file provides comprehensive coverage including:
- Logical plan with cost metrics (rowcount, cumulative cost with cpu/io)
- Physical plan with enumerable operators
- Complex query pattern (aggregation + filter + sort + window function + limit)
This aligns well with testing the Calcite explain cost mode. Based on learnings, this properly tests SQL generation and optimization paths for Calcite integration.
core/src/main/java/org/opensearch/sql/ast/statement/Explain.java (1)
18-30: Clean refactoring to ExplainMode.The changes correctly:
- Replace
ExplainFormatfield withExplainMode- Delegate parsing to
ExplainMode.of(mode)which handles null/empty/invalid inputs- Maintain immutability with
finalfield (appropriate for AST nodes per guidelines)- Preserve the two-constructor pattern for default and explicit mode specification
The null-safe chaining through the overloaded constructor to
ExplainMode.of(null)→STANDARDis a clean default behavior.opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java (1)
39-39: LGTM - Clean migration to ExplainMode.The transition from
ExplainFormattoExplainModeis implemented correctly. The conditional logic properly maps each mode to the appropriateSqlExplainLeveland the control flow for SIMPLE, COST, and EXTENDED modes is maintained.Also applies to: 166-205
core/src/main/java/org/opensearch/sql/executor/execution/QueryPlanFactory.java (2)
77-83: LGTM - Correct migration to ExplainMode.The cursor-based explain path correctly uses
ExplainMode.of(format)to convert the format string to the new mode type.
129-143: LGTM - Consistent with Explain AST node changes.The
visitExplainmethod correctly retrieves the mode from the Explain node vianode.getMode(), which aligns with the updatedExplainclass that now storesExplainMode modeinstead of format.integ-test/src/test/resources/expectedOutput/calcite/explain_output_extended.yaml (1)
1-167: LGTM - Valid test resource for extended explain mode.This test resource correctly captures the expected output structure for the extended explain mode, including:
- Logical plan with Calcite operators
- Physical plan with enumerable operators
- Extended section with generated Java binding code
The content follows expected Calcite Linq4j patterns. As per coding guidelines, the test data appears representative of the explain output schema.
core/src/main/java/org/opensearch/sql/executor/execution/ExplainPlan.java (1)
14-44: LGTM - Clean type migration.The transition from
Explain.ExplainFormattoExplainModeis correctly implemented across the field, constructor, and method signatures.plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryRequest.java (3)
66-78: LGTM - Correct serialization of explainMode.The
explainModefield is properly serialized and deserialized usingwriteOptionalString/readOptionalString, maintaining backward compatibility with null values.Also applies to: 99-110
141-148: LGTM - Correct propagation of explainMode.The
toPPLQueryRequestmethod correctly passes theexplainModeparameter to the newPPLQueryRequestconstructor signature, enabling the mode to be propagated through the request lifecycle.
37-37: No action needed. The null safety concern is unfounded.The code
pplQueryRequest.mode().getModeName()is safe becauseExplainMode.of()always returns a non-null enum constant (either the matched mode orSTANDARDas a fallback), never null. ThegetModeName()getter on the enum will always return a valid string.Likely an incorrect or invalid review comment.
ppl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.java (3)
14-14: LGTM: New ExplainMode import and field.The import and field addition properly support the new explain mode feature.
Also applies to: 28-28
49-67: LGTM: Constructor chaining with explainMode parameter.The constructor properly threads the
explainModethrough, defaulting toSTANDARDfor the 4-argument form.
93-95: No issue found with null handling.The
ExplainMode.of()method already explicitly handles null inputs by returningExplainMode.STANDARDon line 22 (if (mode == null || mode.isEmpty()) return STANDARD;). Themode()method is safe and will not throw an NPE for non-explain requests.Likely an incorrect or invalid review comment.
integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java (1)
29-29: LGTM: Updated imports and endpoint template.The new
EXPLAIN_API_ENDPOINTtemplate with format and mode parameters consolidates the previous separate endpoints.Also applies to: 33-33, 38-38
plugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.java (2)
22-26: LGTM: New constants for explain mode support.The constants are appropriately named and follow the existing pattern for query parameters.
62-62: LGTM: Clean integration of explainMode in request parsing.The
explainModeis extracted and passed toPPLQueryRequestfollowing the same pattern as other parameters.Also applies to: 76-76
core/src/main/java/org/opensearch/sql/executor/QueryService.java (3)
29-29: LGTM: Import updated to ExplainMode.The import change aligns with the refactoring from
ExplainFormattoExplainMode.
79-90: LGTM: Updated explain method signature.The method cleanly delegates to Calcite or legacy paths while propagating the
ExplainMode.
133-167: LGTM: Calcite explain path updated with mode.The
ExplainModeis correctly propagated to the execution engine and through the fallback path.
Signed-off-by: Heng Qian <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.java (1)
62-62: Consider adding test coverage for other ExplainMode values.The PR introduces multiple explain modes (SIMPLE, STANDARD, EXTENDED, COST), but tests only exercise
STANDARD. Consider parameterizing the explain tests to cover different modes, ensuring theQueryService.explain()method correctly handles all supported values.♻️ Example using JUnit 5 parameterized test
@ParameterizedTest @EnumSource(ExplainMode.class) public void testExplainWithDifferentModes(ExplainMode mode) { queryService().explainSuccess().handledByExplainOnResponse(mode); }This would require adjusting
handledByExplainOnResponseto accept a mode parameter.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.javadocs/user/ppl/interfaces/endpoint.mdinteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
✅ Files skipped from review due to trivial changes (1)
- docs/user/ppl/interfaces/endpoint.md
🧰 Additional context used
📓 Path-based instructions (6)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
**/*Test.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*Test.java: All new business logic requires unit tests
Name unit tests with*Test.javasuffix in OpenSearch SQL
Files:
core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
integ-test/**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
End-to-end scenarios need integration tests in
integ-test/module
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
⚙️ CodeRabbit configuration file
integ-test/**/*IT.java: - Integration tests MUST use valid test data from resources
- Verify test data files exist in integ-test/src/test/resources/
- Check test assertions are meaningful and specific
- Validate tests clean up resources after execution
- Ensure tests are independent and can run in any order
- Flag tests that reference non-existent indices (e.g., EMP)
- Verify integration tests are in correct module (integ-test/)
- Check tests can be run with ./gradlew :integ-test:integTest
- Ensure proper test data setup and teardown
- Validate end-to-end scenario coverage
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Name integration tests with
*IT.javasuffix in OpenSearch SQL
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
**/calcite/**/*.java
⚙️ CodeRabbit configuration file
**/calcite/**/*.java: - Follow existing Calcite integration patterns
- Verify RelNode visitor implementations are complete
- Check RexNode handling follows project conventions
- Validate SQL generation is correct and optimized
- Ensure Calcite version compatibility
- Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
🧠 Learnings (5)
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Applied to files:
core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*Test.java : Name unit tests with `*Test.java` suffix in OpenSearch SQL
Applied to files:
core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*.java : Document Calcite-specific workarounds in code
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
📚 Learning: 2025-12-29T05:32:03.491Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:03.491Z
Learning: For any custom Calcite RelNode class (e.g., ones that extend EnumerableLimitSort or other Calcite RelNode types), always override the copy method. If copy is not overridden, cloning/copy operations may downgrade the instance to the parent class type, losing the custom behavior. In your implementation, ensure copy returns a new instance of the concrete class with all relevant fields and traits preserved, mirroring the current instance state.
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
🧬 Code graph analysis (1)
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java (1)
common/src/main/java/org/opensearch/sql/common/setting/Settings.java (1)
Settings(17-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (8)
core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.java (3)
23-23: LGTM!Import correctly updated to use the new
ExplainModeenum, aligning with the PR's objective to separate explain mode from format parameters.
210-226: LGTM!The method correctly passes the
modeparameter toqueryService.explain(), and the response listener properly validates the successful explain response.
228-244: LGTM!The failure path correctly uses the
modeparameter and properly validates thatIllegalStateExceptionis caught by the failure handler.integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java (5)
28-29: LGTM!The new imports properly support the PR's objective of separating explain mode from format. The
ExplainModeenum replaces the previous format-based explain handling, and theKeyimport simplifies the settings references.
1723-1735: LGTM!The simplified
Key.PPL_SYNTAX_LEGACY_PREFERREDreference is cleaner and consistent with the new static import.
2209-2219: LGTM!The test now explicitly specifies
ExplainMode.EXTENDEDrather than relying on implicit format-based mode selection. This makes the test intent clearer and aligns with the PR's separation of concerns.
381-389: LGTM! Demonstrates the new mode/format separation.The test correctly uses
explainQueryYaml(query, ExplainMode.EXTENDED)to specify both the output format (YAML) and explain verbosity (EXTENDED) independently, aligning with the PR objective. The expected YAML resource file exists in the resources directory.
2322-2348: Good comprehensive test coverage for the new mode/format API.This test effectively validates the separation of explain mode from format by testing all four
ExplainModevalues (SIMPLE, STANDARD, EXTENDED, COST) with both YAML and JSON output formats. The query covers a good variety of PPL commands, and all 8 expected resource files are present ininteg-test/src/test/resources/expectedOutput/calcite/.
Signed-off-by: Heng Qian <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanTest.java (1)
112-128: LGTM! Error handling test correctly updated for new parameter.The test properly validates that explain with pagination throws
NotImplementedExceptionand now correctly passes themodeparameter.💡 Optional: Consider using assertInstanceOf (JUnit 5.8+)
If the project uses JUnit 5.8+,
assertInstanceOfprovides better error messages:- assertTrue(e instanceof NotImplementedException); + assertInstanceOf(NotImplementedException.class, e);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanTest.java
🧰 Additional context used
📓 Path-based instructions (3)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanTest.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanTest.java
**/*Test.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*Test.java: All new business logic requires unit tests
Name unit tests with*Test.javasuffix in OpenSearch SQL
Files:
core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanTest.java
🧠 Learnings (1)
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: Update draft release notes
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: test-sql-cli-integration (21)
- GitHub Check: enforce-label
🔇 Additional comments (2)
core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanTest.java (2)
23-23: LGTM! Import and mock field correctly updated to use new ExplainMode type.The migration from
Explain.ExplainFormattoExplainModeis consistent with the PR objective of separating explain verbosity from response format.Also applies to: 48-48
58-64: LGTM! Test properly updated to verify new ExplainMode parameter.The test correctly verifies that
QueryService.explain()is invoked with the newmodeparameter, and the verification matches the expected method signature.
Signed-off-by: Heng Qian <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/test/java/org/opensearch/sql/executor/execution/CommandPlanTest.java (1)
66-78: Consider using an actual enum value instead of mocking.The API update from
ExplainFormattoExplainModeis correctly applied. However, mocking an enum is generally fragile and unnecessary here—since the test only verifies thatexplain()throws regardless of the mode value, using an actual enum constant would be simpler and more robust.♻️ Suggested refactor
- ExplainMode mode = mock(ExplainMode.class); + ExplainMode mode = ExplainMode.STANDARD;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/test/java/org/opensearch/sql/executor/execution/CommandPlanTest.java
🧰 Additional context used
📓 Path-based instructions (3)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
core/src/test/java/org/opensearch/sql/executor/execution/CommandPlanTest.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
core/src/test/java/org/opensearch/sql/executor/execution/CommandPlanTest.java
**/*Test.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*Test.java: All new business logic requires unit tests
Name unit tests with*Test.javasuffix in OpenSearch SQL
Files:
core/src/test/java/org/opensearch/sql/executor/execution/CommandPlanTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
core/src/test/java/org/opensearch/sql/executor/execution/CommandPlanTest.java
🧠 Learnings (2)
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
core/src/test/java/org/opensearch/sql/executor/execution/CommandPlanTest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*Test.java : Name unit tests with `*Test.java` suffix in OpenSearch SQL
Applied to files:
core/src/test/java/org/opensearch/sql/executor/execution/CommandPlanTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (21, integration)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: build-linux (25, unit)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: security-it-linux (25)
- GitHub Check: security-it-linux (21)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (1)
core/src/test/java/org/opensearch/sql/executor/execution/CommandPlanTest.java (1)
22-22: LGTM!Import correctly updated to reflect the new
ExplainModetype replacing the oldExplainFormat.
|
|
||
| To translate your query, send it to explain endpoint. The explain output is OpenSearch domain specific language (DSL) in JSON format. You can just copy and paste it to your console to run it against OpenSearch directly. | ||
| Explain output could be set different formats: `standard` (the default format), `simple`, `extended`, `dsl`. | ||
| Explain API supports various mode: `standard` (the default format), `simple`, `extended`, `cost`. |
There was a problem hiding this comment.
+1, mode is more clear then format.
Q, PPL should support format=[simple, extended, cost] until 4.x, then we can deprecated it, right? If yes, we should add deprecated warnning info in resposne.
There was a problem hiding this comment.
Do you mean we should keep backward compatible? The current code won't support format=[simple, extended, cost] anymore.
There was a problem hiding this comment.
+1 to add format=[simple, extended, cost] as alias of mode=[simple, extended, cost] before 4.x and clarify them deprecated, even they are not wildly used, but as a policy of open source community, we don't want to introduce API level breaking changes.
There was a problem hiding this comment.
Addressed in 17e9334, format=[simple, standard, extended, cost] is supported as well.
Signed-off-by: Heng Qian <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@plugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.java`:
- Around line 62-71: When Format.isExplainMode(format) is true you must emit a
deprecation warning instead of silently switching behavior: add a private static
final Logger LOG = LogManager.getLogger(PPLQueryRequestFactory.class) to the
class and, inside the branch where you set explainMode = format.getFormatName()
and format = Format.JSON, call LOG.warn(...) with a clear deprecation message
mentioning the legacy format parameter (e.g., include format.getFormatName() and
restRequest.param("format") or restRequest.rawPath() for context) so users are
notified that format-based explain modes are deprecated and will be removed
after 4.x.
♻️ Duplicate comments (2)
integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java (2)
62-66: Inconsistent deprecation notice.The
@Deprecatedcomment points toexplainQueryYaml(String), but this deprecated method returns JSON format. Consider updating to point toexplainQueryToString(String, ExplainMode)instead.
80-86: Use enum getter methods instead of relying ontoString().Line 83 passes
formatandmodedirectly toString.format(), which invokestoString()returning uppercase enum names (e.g., "JSON", "STANDARD"). Useformat.getFormatName()andmode.getModeName()to get the correct lowercase values expected by the API.🔧 Suggested fix
private String explainQuery(String query, Format format, ExplainMode mode) throws IOException { Response response = client() - .performRequest(buildRequest(query, String.format(EXPLAIN_API_ENDPOINT, format, mode))); + .performRequest(buildRequest(query, String.format(EXPLAIN_API_ENDPOINT, format.getFormatName(), mode.getModeName()))); Assert.assertEquals(200, response.getStatusLine().getStatusCode()); return getResponseBody(response, true); }
🧹 Nitpick comments (1)
integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java (1)
88-94: Useformat.getFormatName()for consistency.Similar to the
explainQuerymethod, line 91 should useformat.getFormatName()instead of relying on the enum'stoString()method.♻️ Suggested fix
protected String explainQueryToStringBWC(String query, Format format) throws IOException { Response response = client() - .performRequest(buildRequest(query, String.format(BWC_EXPLAIN_API_ENDPOINT, format))); + .performRequest(buildRequest(query, String.format(BWC_EXPLAIN_API_ENDPOINT, format.getFormatName()))); Assert.assertEquals(200, response.getStatusLine().getStatusCode()); return getResponseBody(response, true).replace("\\r\\n", "\\n"); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.javainteg-test/src/test/java/org/opensearch/sql/util/MatcherUtils.javaplugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.javaprotocol/src/main/java/org/opensearch/sql/protocol/response/format/Format.java
🚧 Files skipped from review as they are similar to previous changes (1)
- integ-test/src/test/java/org/opensearch/sql/util/MatcherUtils.java
🧰 Additional context used
📓 Path-based instructions (6)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
protocol/src/main/java/org/opensearch/sql/protocol/response/format/Format.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javaplugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.javainteg-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
protocol/src/main/java/org/opensearch/sql/protocol/response/format/Format.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javaplugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.javainteg-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java
integ-test/**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
End-to-end scenarios need integration tests in
integ-test/module
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
⚙️ CodeRabbit configuration file
integ-test/**/*IT.java: - Integration tests MUST use valid test data from resources
- Verify test data files exist in integ-test/src/test/resources/
- Check test assertions are meaningful and specific
- Validate tests clean up resources after execution
- Ensure tests are independent and can run in any order
- Flag tests that reference non-existent indices (e.g., EMP)
- Verify integration tests are in correct module (integ-test/)
- Check tests can be run with ./gradlew :integ-test:integTest
- Ensure proper test data setup and teardown
- Validate end-to-end scenario coverage
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Name integration tests with
*IT.javasuffix in OpenSearch SQL
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java
**/calcite/**/*.java
⚙️ CodeRabbit configuration file
**/calcite/**/*.java: - Follow existing Calcite integration patterns
- Verify RelNode visitor implementations are complete
- Check RexNode handling follows project conventions
- Validate SQL generation is correct and optimized
- Ensure Calcite version compatibility
- Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
**/ppl/**/*.java
⚙️ CodeRabbit configuration file
**/ppl/**/*.java: - For PPL parser changes, verify grammar tests with positive/negative cases
- Check AST generation for new syntax
- Ensure corresponding AST builder classes are updated
- Validate edge cases and boundary conditions
Files:
integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java
🧠 Learnings (4)
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
📚 Learning: 2025-12-29T05:32:03.491Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:03.491Z
Learning: For any custom Calcite RelNode class (e.g., ones that extend EnumerableLimitSort or other Calcite RelNode types), always override the copy method. If copy is not overridden, cloning/copy operations may downgrade the instance to the parent class type, losing the custom behavior. In your implementation, ensure copy returns a new instance of the concrete class with all relevant fields and traits preserved, mirroring the current instance state.
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
📚 Learning: 2026-01-13T17:39:04.107Z
Learnt from: ykmr1224
Repo: opensearch-project/sql PR: 5028
File: integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java:23-35
Timestamp: 2026-01-13T17:39:04.107Z
Learning: In OpenSearch SQL integration tests that extend PPLIntegTestCase or SQLIntegTestCase, index cleanup is managed by the parent test infrastructure. Do not rely on per-test After methods to delete indices created during tests; rely on the base class teardown to clean up. This applies to tests extending those base classes and is not needed for tests that do not extend them.
Applied to files:
integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java
🧬 Code graph analysis (2)
protocol/src/main/java/org/opensearch/sql/protocol/response/format/Format.java (1)
core/src/main/java/org/opensearch/sql/ast/statement/Explain.java (1)
Getter(14-36)
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java (1)
common/src/main/java/org/opensearch/sql/common/setting/Settings.java (1)
Settings(17-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: security-it-linux (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (21, unit)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (21, integration)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: build-linux (25, doc)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (14)
plugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.java (3)
22-26: LGTM!Constants are well-named following
UPPER_SNAKE_CASEconvention and align with the PR objectives for separating explain mode from format. Default values are sensible.
146-153: Note that invalid explain mode values silently default to STANDARD rather than failing explicitly.While the method appropriately delegates validation to
ExplainMode.of(), which handles the string-to-enum conversion, the error handling is lenient: invalid modes are silently converted toSTANDARDrather than throwing an exception with a clear error message. This fail-safe approach is acceptable but should be documented or considered for explicit validation if strict mode validation is preferred.
79-86: No action required. The code properly handles nullexplainModevalues.ExplainMode.of()explicitly checks for null and empty strings, returningExplainMode.STANDARDas the default, ensuring downstream consumers receive a valid enum value.protocol/src/main/java/org/opensearch/sql/protocol/response/format/Format.java (3)
22-33: LGTM! Clean separation of explain output formats from explain modes.The new enum values
JSONandYAMLfor output serialization are distinct from the existingSIMPLE,STANDARD,EXTENDED,COSTmodes for explain verbosity. This aligns well with the PR objective of separating these concerns.
41-42: LGTM! Proper immutable set for explain modes.Using
Set.of()ensures immutability, and the grouping correctly includes only the verbosity modes (SIMPLE, STANDARD, EXTENDED, COST) without the output formats (JSON, YAML).Also applies to: 60-61
69-76: LGTM! Default output format and mode check method.The default change from
"standard"to"json"aligns with the new separation where format controls serialization (defaulting to JSON). TheisExplainMode()helper cleanly determines if a format value represents a legacy explain mode.integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java (2)
38-39: LGTM! Well-structured endpoint patterns.The separation of
BWC_EXPLAIN_API_ENDPOINT(backward-compatible with just format) andEXPLAIN_API_ENDPOINT(new mode-aware with both format and mode) provides clean support for both legacy and new API patterns.
68-78: LGTM! Clean delegation pattern for mode-aware explain methods.The overloaded methods provide a clear API surface with consistent delegation to the private
explainQueryhelper.integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java (6)
28-31: LGTM! Imports added for new mode/format handling.The imports for
ExplainMode,Settings.Key, andFormatsupport the new explain mode functionality being tested.
387-390: LGTM! Test correctly uses new mode-aware API.The test properly uses
explainQueryYaml(query, ExplainMode.EXTENDED)to request extended explain output in YAML format, with a matching YAML expected file.
1725-1726: LGTM! Consistent use of imported Key enum.The shorthand
Key.PPL_SYNTAX_LEGACY_PREFERREDis cleaner and consistent with the new import at line 29.Also applies to: 1751-1752
2212-2219: LGTM! Test updated for YAML output with EXTENDED mode.The test correctly uses the new mode-aware API and matches the expected output format.
2351-2370: Good backward compatibility test coverage.This test validates that the legacy format-based API continues to work correctly for all explain modes. It properly reuses the same expected JSON files as
testExplainInVariousModeAndFormat, ensuring consistent behavior between new and legacy APIs.
2324-2349: Good comprehensive test coverage for all mode/format combinations.This test verifies all four explain modes (SIMPLE, STANDARD, EXTENDED, COST) with both YAML and JSON output formats. The use of
mode.getModeName().toLowerCase()for file naming is correct and consistent. All required test data files exist ininteg-test/src/test/resources/expectedOutput/calcite/.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Signed-off-by: Heng Qian <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
protocol/src/test/java/org/opensearch/sql/protocol/response/format/FormatTest.java (1)
46-52: Consider expanding test coverage forisExplainMode.The assertion for
isExplainModeis a good addition. However, this only tests one explain mode (EXTENDED). Consider adding a dedicated test method that verifiesisExplainModereturnstruefor all explain modes (SIMPLE, STANDARD, EXTENDED, COST) andfalsefor serialization formats (JSON, YAML, CSV, JDBC, RAW).💡 Suggested additional test
`@Test` void isExplainModeIdentifiesCorrectFormats() { // Explain modes should return true assertTrue(Format.isExplainMode(Format.SIMPLE)); assertTrue(Format.isExplainMode(Format.STANDARD)); assertTrue(Format.isExplainMode(Format.EXTENDED)); assertTrue(Format.isExplainMode(Format.COST)); // Serialization formats should return false assertFalse(Format.isExplainMode(Format.JSON)); assertFalse(Format.isExplainMode(Format.YAML)); assertFalse(Format.isExplainMode(Format.CSV)); assertFalse(Format.isExplainMode(Format.JDBC)); assertFalse(Format.isExplainMode(Format.RAW)); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.javaprotocol/src/test/java/org/opensearch/sql/protocol/response/format/FormatTest.java
🧰 Additional context used
📓 Path-based instructions (3)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
protocol/src/test/java/org/opensearch/sql/protocol/response/format/FormatTest.javaplugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
protocol/src/test/java/org/opensearch/sql/protocol/response/format/FormatTest.javaplugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.java
**/*Test.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*Test.java: All new business logic requires unit tests
Name unit tests with*Test.javasuffix in OpenSearch SQL
Files:
protocol/src/test/java/org/opensearch/sql/protocol/response/format/FormatTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
protocol/src/test/java/org/opensearch/sql/protocol/response/format/FormatTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (25, unit)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (25, doc)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: security-it-linux (21)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (5)
protocol/src/test/java/org/opensearch/sql/protocol/response/format/FormatTest.java (1)
61-66: LGTM!The change from
Format.STANDARDtoFormat.JSONcorrectly reflects the new separation of concerns. When no format is specified for explain, defaulting to JSON (a serialization format) rather than STANDARD (an explain verbosity mode) aligns with the PR objective to decouple these responsibilities.plugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.java (4)
10-11: LGTM!Logger is correctly declared following Java conventions and is appropriately used for the deprecation warning.
Also applies to: 21-22
26-26: LGTM!Constants follow
UPPER_SNAKE_CASEnaming convention and align with the PR objectives for separating mode from format.Also applies to: 29-30
87-94: LGTM!The
explainModeparameter is correctly propagated to thePPLQueryRequestconstructor.
154-161: Validation for explain modes already exists in theExplainMode.of()method, which is called downstream whenPPLQueryRequest.mode()processes the mode string. Invalid modes are gracefully handled by defaulting toSTANDARDrather than throwing an exception. The current design is intentional and provides better user experience.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
plugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.java
Show resolved
Hide resolved
|
The backport to To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.19-dev 2.19-dev
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.19-dev
# Create a new branch
git switch --create backport/backport-5042-to-2.19-dev
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 5dbcd285fc6f05620826c2664abda2cff21a4245
# Push it to GitHub
git push --set-upstream origin backport/backport-5042-to-2.19-dev
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.19-devThen, create a pull request where the |
* Separate explain mode from format params and support different combination Signed-off-by: Heng Qian <[email protected]> * Fix CI Signed-off-by: Heng Qian <[email protected]> * Fix CI Signed-off-by: Heng Qian <[email protected]> * Fix CI Signed-off-by: Heng Qian <[email protected]> * Support explain format BWC Signed-off-by: Heng Qian <[email protected]> * Address comments and increase test coverage Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]> (cherry picked from commit 5dbcd28) Signed-off-by: Heng Qian <[email protected]>
* Separate explain mode from format params and support different combination * Fix CI * Fix CI * Fix CI * Support explain format BWC * Address comments and increase test coverage --------- (cherry picked from commit 5dbcd28) Signed-off-by: Heng Qian <[email protected]>
4 participants
Description
Introduce a new query parameter
mode(allowsSIMPLE,STANDARD,EXTENDED,COST) specifically for controlling the explain verbosity, while keeping format strictly for output serialization format(allowsJSON,YAMLfor explain API or command).e.g.
Before this PR, when setting
format=yaml, users cannot specify the explain mode anymore with the explain mode bounded to the defaultSTANDARD.For backward compatibility, it also supports
format=[simple, standard, extended, cost]and get the same behavior as beforeRelated Issues
Resolves #4940
Check List
--signoffor-s.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.